phd: check exit status of commands #1054
Conversation
similar to std::process::Command: if a command fails with status, we should report that more proactively. further, some commands are expected to exit with status in tests, so adjust test expectations to match the new regime.
| // whether the guest thinks it's running on a Hyper-V-compatible | ||
| // hypervisor. (Whether any actual enlightenments are enabled is another | ||
| // story, but those can often be detected by other means.) | ||
| let out = vm.run_shell_command("systemd-detect-virt").await?; |
There was a problem hiding this comment.
with this change, the shell command completes with status, so this change ends up being a motivator to detect the hypervisor in a more robust way. yay!
|
THANK YOU |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
this is great, let's get this merged so i can stop being scared all our PHD tests are passing by mistake
| self.vm.guest_os.shell_command_sequence(self.cmd); | ||
| self.vm.run_command_sequence(command_sequence).await?; |
There was a problem hiding this comment.
hmm, it occurs to me that this could maybe be jammed together into a method on self.vm, not that we are currently calling it anywhere else...
| // reported in dmesg either it's genuinely not detected, it's a very old | ||
| // Linux, or it's a new Linux and dmesg text has changed. |
There was a problem hiding this comment.
i suppose it could possibly also have fallen off the end of the dmesg ringbuffer if something is spewing out a ton of errors perhaps?
There was a problem hiding this comment.
oh god. file that under "conditions i never want to think about". but yes.
| // For reasons absolutely indecipherable to me, Alpine (3.16, kernel | ||
| // 5.15.41-0-virt) seems to lose some early dmesg lines if booted with less | ||
| // than four vCPUs. Among the early dmesg lines are `Hypervisor detected: | ||
| // Microsoft Hyper-V` that we look for as confirmation that Linux knows | ||
| // there's a Hyper-V-like hypervisor present. |
| // the file should still exist on the target VM after migration. | ||
| let lsout = target | ||
| .run_shell_command("ls foo.bar") | ||
| .ignore_status() |
There was a problem hiding this comment.
since we're asserting on the output of ls below, it's mildly more interesting to try ls and get whatever it did print in the error w/ assert_eq!() instead of "ls exited with 1 but we don't know anything more about it".
this isn't super important but iirc i'd broken the test in a really confusing way where this helped explain what was happening, rather than just saying the right thing wasn't happening.
2 participants
similar to std::process::Command: if a command fails with status, we should report that more proactively. further, some commands are expected to exit with status in tests, so adjust test expectations to match the new regime.
a billion years ago I opened #797, then sixish months ago I closed 797 with the intent to refresh (.. rebase) it on a current Propolis commit. last week we found out I forgot to actually do that, and so this actually does it! let's get this in this time, instead of not lol
I'm a little surprised that this built cleanly, and I cannot currently run phd locally for Weird bhyve Issues I Need To Go Debug, so I'm leaning on CI for the check in
phd-run.